Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default accounts in integration test implementation #1955

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

ramirez7358
Copy link

@ramirez7358 ramirez7358 commented Oct 27, 2023

Summary

Closes #1953

  • [n] y/n | Does it introduce breaking changes?
  • [n] y/n | Is it dependant on the specific version of cargo-contract or pallet-contracts?

Description

Function

default_accounts()

Initial Status

Implementation Difference

Issue Description

Default accounts addresses do not match across integration and e2e tests.

Issue Documentation & Test Case

https://github.com/CoinFabrik/on-ink-integration-tests/tree/main/test-cases/default-accounts

Current Status

Implementation Difference Corrected. Test Cases in Target Repo Passed. Pull Request Performed to Corresponding Repository.

Implementation Notes

Now the integration tests mimic the account setup in e2e tests. We changed the name of the accounts "Django" to "Dave" and "Frank" to "Ferdie". On the other hand, there were two accounts in e2e that did not exist in integration tests, accounts “one” and “two”. We added these accounts to integration tests.

Moreover, since e2e tests were drawing these accounts from the library sp_keyring::sr25519::Keyring, we made integration tests depend on the same library in order to account for future changes in this lib.

Updated Documentation

We updated the original documentation of this issue in our repository, adding the section Update on Correcting this Issue and informing of the correction.

In the target repository, we updated the inline documentation. The associated documentation should be updated automatically in these pages: ink_env test default_accounts, ink_env test DefaultAccounts, ink_e2e AccountKeyring.

Testing Guide

In the directory integration-tests/default_accounts of the target directory, we include in our pull request a test case showing that the observed implementation difference has been corrected. Note that this test is different from the original test case in our repo, which showed the implementation difference.

In this directory integration-tests/default_accounts, run the following command to run both integration and e2e tests:
cargo test -–features e2e-tests

These tests are run in the same function in order to compare the results of both environments.

Checklist before requesting a review

  • My code follows the style guidelines of this project
  • I have added an entry to CHANGELOG.md
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

@SkymanOne SkymanOne mentioned this pull request Nov 30, 2023
@cmichi
Copy link
Collaborator

cmichi commented Nov 14, 2024

@ramirez7358 Could you merge master into this PR and resolve the conflicts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default accounts addresses do not match across integration and e2e tests.
4 participants